-
Notifications
You must be signed in to change notification settings - Fork 850
Add InvocationRequired property to FunctionCallContent #7126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
Show resolved
Hide resolved
|
@copilot, fix the CI failures |
Fixed in da8a275. The tests were reusing FunctionCallContent objects across multiple test iterations. When we set InvocationRequired=false during processing, subsequent iterations saw them as already processed. Added a ResetPlanFunctionCallStates helper method that resets InvocationRequired=true at the beginning of test helper methods. All 385 AI tests and 1115 abstraction tests now pass. |
|
@javiercn, @jozkee, @westey-m, thoughts on this approach? The goal here is to:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR adds an InvocationRequired property to FunctionCallContent to prevent duplicate processing of function calls in the FunctionInvokingChatClient. The property defaults to true and is set to false after a function call has been processed, allowing the middleware to distinguish between new function calls and already-processed ones. This is particularly useful in scenarios where chat history containing function calls is reused across multiple requests.
Key changes:
- Added
InvocationRequiredboolean property toFunctionCallContentwith JSON serialization support (omitted when false) - Modified
FunctionInvokingChatClientto set the property to false after processing and filter out function calls withInvocationRequired = false - Added comprehensive test coverage including unit tests for serialization behavior and integration tests for filtering logic
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs | Adds InvocationRequired property with field initializer defaulting to true, marked as experimental, with JsonIgnoreCondition.WhenWritingDefault to omit when false |
| src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs | Updates CopyFunctionCalls to filter out function calls where InvocationRequired = false and sets the property to false in ProcessFunctionCallAsync and CreateFunctionResultContent |
| test/Libraries/Microsoft.Extensions.AI.Abstractions.Tests/Contents/FunctionCallContentTests.cs | Adds 6 unit tests covering property defaults, getters/setters, and JSON serialization/deserialization behavior |
| test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs | Adds 4 integration tests for the filtering behavior and a ResetPlanFunctionCallStates helper method to reset the property when reusing test plans across multiple invocations |
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Outdated
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI/ChatCompletion/FunctionInvokingChatClient.cs
Show resolved
Hide resolved
test/Libraries/Microsoft.Extensions.AI.Tests/ChatCompletion/FunctionInvokingChatClientTests.cs
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Outdated
Show resolved
Hide resolved
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
Assuming we will always have a The representation of a function that was already invoked already exists as the I suggest that we might just have a nullable Further downstream Often I found myself having to store the |
Could we instead look for the matching I think this goes in line with what @rogerbarreto has in mind. |
|
@javiercn does AGUI always returns a matching FRC for a server-side function? |
We could, it's just fairly complex, especially in a streaming scenario where you may have received the FCC but not yet the FRC, but even in non-streaming they can (and typically) are spread across multiple messages. We would need to employ that logic in FunctionInvokingChatClient, and any consumer that wanted to handle function invocation manually (often for a subset of requests) would need to have similar logic (we could try to expose something in a helper?) We would also need to mandate that any FCC that should be considered handled must always have a paired FRC. I'd tried this approach in #7011 but gave up on it due to complexity. We can experiment with it again, though. |
|
#7141 is an attempt at handling it based on looking for corresponding FRCs in FunctionInvokingChatClient. |
|
I created this issue to see if we could use that in links in code and docs around this issue, but also have a central place to get comments/discussions that are not directly related to a specific PR. #7204 |
Co-authored-by: stephentoub <2642209+stephentoub@users.noreply.github.com>
src/Libraries/Microsoft.Extensions.AI.Abstractions/Microsoft.Extensions.AI.Abstractions.json
Show resolved
Hide resolved
src/Libraries/Microsoft.Extensions.AI.Abstractions/Contents/FunctionCallContent.cs
Show resolved
Hide resolved
6f38322 to
30d0ee7
Compare
Implementation Summary
Successfully implemented the
InvocationRequiredproperty forFunctionCallContentwith the following features:Add
InvocationRequiredproperty toFunctionCallContentclasstrue(requires invocation)true(from field initializer)Microsoft.Extensions.AI.Abstractions.jsonfileUpdate
FunctionInvokingChatClientto setInvocationRequired = falsewhen handling functionProcessFunctionCallAsyncbefore any processingUpdate
FunctionInvokingChatClientto filter out already-processed function callsCopyFunctionCallsto skipFunctionCallContentwhereInvocationRequired == falseComprehensive test coverage (1505 tests passing)
FunctionCallContentTests.csfor property behavior and JSON serializationFunctionInvokingChatClientTests.csfor integration behaviorFixed CI failures
ResetPlanFunctionCallStateshelper method to resetInvocationRequiredstateAll code review feedback addressed
Goals Achieved
This implementation enables:
Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.
Microsoft Reviewers: Open in CodeFlow